Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add migration configuration and basic migration. #213

Merged
merged 10 commits into from
Jul 26, 2023

Conversation

kinyoklion
Copy link
Member

@kinyoklion kinyoklion commented Jul 25, 2023

This does not have consistency tracking, error tracking, or latency tracking.

Most documentation is temporary.

/**
* Configuration class for configuring serial execution of a migration.
*/
export class LDSerialExecution {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not 100% on these being classes. But it can be easily changed if it proves cumbersome to use from JS.

@kinyoklion kinyoklion marked this pull request as ready for review July 25, 2023 20:52
@kinyoklion kinyoklion requested review from keelerm84 and removed request for yusinto July 25, 2023 20:53
fromNew: LDMethodResult<TMigrationRead>;
};

async function safeCall<TResult>(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't trust people to not have exceptions, so this handles an exception and turns it into an error return.

*
* An implementation may also throw an exception to represent an error.
*/
export type LDMethodResult<TResult> =
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've gone with a rust-like tagged union.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I'm not going to add monadic operation.)

});

describe.each([
[new LDSerialExecution(LDExecutionOrdering.Fixed), 'serial fixed'],
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have builders, but these "execution" classes ensure correct combinations without having a potentially unused config at the top level.

])(
'uses the correct authoritative source: %p, read: %p, write: %p.',
async (stage, readValue, writeResult) => {
const migration = new Migration<string, boolean>(client, {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The location of the Migration type may not be permanent yet, and I need to decide if you directly construct one, or if we have a factory. The SDK has not exposed any implementation types before.

Copy link
Member

@keelerm84 keelerm84 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good. One point for consideration, but nothing that couldn't be addressed in a follow-up.

* Results from `readOld` or `writeOld` will be 'old'.
* Results from `readNew` or `writeNew` will be 'new'.
*/
export type LDMigrationOrigin = 'old' | 'new';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the spec, I was thinking it might be useful to designate whether they were old or new sources, as well as if they were authoritative or not. This way the caller doesn't have to encode our same mapping table, and we can more easily change it in the future. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I see how, at least for writes, that information could provide some value. I will try it.

@kinyoklion kinyoklion merged commit b48f847 into feat/node-migrations Jul 26, 2023
13 checks passed
@kinyoklion kinyoklion deleted the rlamb/migrator-configuration branch July 26, 2023 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants